Replace internal dependencies by FetchContent #1583

DerNils-git · 2024-01-06T10:53:17Z

Replace internal dependencies management by CMake FetchContent commands.

Advantage:

Updating the dependencies is simply done by updating the version/hash used within FetchContent_Declare which makes it easier to stay at the project HEAD of dependencies for project maintainers
Less CMake options that users have to take care about while keeping the same flexibility

Required changes:

Increase CMake minimum required version

Dependencies still have to be updated:

TOML11 requires CMAKE_CXX_STANDARD to be set if the latest hash shall be used
Catch2 requires to be moved from v2 to v3 which requires additional source changes.

If the pull request is accepted I would open issues to update the dependencies mentioned above and assign them to me.

(Sorry about the slightly chaotic git history. The change to FetchContent is done from 0ba14dc to 0084e9b. The previous commits resulted from a previous try to move to FetchContent which failed and the commits are reverted)

…ace library for nlohmann_json

This reverts commit 41a4658.

…D interface library for nlohmann_json" This reverts commit 6136a97.

This reverts commit 620d861.

Add the directory filled by FetchContent to .gitignore Add CMakeUserPresets.json to .gitignore

…ybind11 configuration.

for more information, see https://pre-commit.ci

franzpoeschel · 2024-01-12T10:43:49Z

.gitignore

+share/openPMD/thirdParty/json/
+share/openPMD/thirdParty/toml11/
+share/openPMD/thirdParty/catch2/
+share/openPMD/thirdParty/pybind11/


We should be aware that with this, the build folder will modify the source tree.
What does the workflow look like switching between different openPMD versions that specify different dependency versions. Will FetchContent notice and update the dependencies' versions upon configuring?

That is true and might be a problem. I don't have experience with this.

Moving from lower to higher versions of OpenPMD should not be a problem. These directories have been deleted using git rm and should therefore be deleted when moving forward. FetchContent can then download into these directories w/o problem.

A problem might arise if one moves from a higher to a lower version. If the directories have been populated by FetchContent and git tries to add the old libraries again in the same directories I can not predict the behavior.

It would be possible to use a different directory with FetchContent until we are sure that it works as expected.

Since @ax3l uses FetchContent in WarpX, let's wait until he is back to see what their approach looks like (relevant bits in their CMake config: https://github.com/ECP-WarpX/WarpX/blob/11e2a1722aac8929db0ed677f634673d235c1396/cmake/dependencies/openPMD.cmake#L29)

I'll move this to the build directory, as we do in other project.

franzpoeschel · 2024-01-12T10:45:21Z

CMakeLists.txt

+      toml11
+      GIT_REPOSITORY https://github.com/ToruNiina/toml11
+      # Migrate to the latest commit to remove CMake Warning which is not yet
+      # available in any official release.


You mean the warning below? Do I understand correctly that this has been fixed on toml11 dev and we are waiting for the next release?

CMake Deprecation Warning at share/openPMD/thirdParty/toml11/CMakeLists.txt:1 (cmake_minimum_required): Compatibility with CMake < 3.5 will be removed from a future version of CMake. Update the VERSION argument <min> value or use a ...<max> suffix to tell CMake that the project does not need compatibility with older versions.

Yes.
But also moving to the last commit is not trivial with toml11 since that requires CMAKE_CXX_STANDARD to be set which I could not add to openPMD-api w/o breaking the CI.

since that requires CMAKE_CXX_STANDARD to be set which I could not add to openPMD-api w/o breaking the CI.

That's probably an issue unrelated to this PR which we should better solve separately?

Yes, after finishing this PR I would try to Update catch2 and toml11 with separate issues as described above

CMakeLists.txt

README.md

Co-authored-by: Franz Pöschel <[email protected]>

DerNils-git · 2024-01-17T09:04:24Z

Is it to be expected that one of the CI stages fails? The error in this step does not seem to be related to the changes of this pull request.

franzpoeschel · 2024-01-17T14:31:28Z

Is it to be expected that one of the CI stages fails? The error in this step does not seem to be related to the changes of this pull request.

The failure comes from the increased minimum CMake version:

CMake Error at CMakeLists.txt:3 (cmake_minimum_required):
  CMake 3.24.0 or higher is required.  You are running version 3.22.1

The other "red text" seems to be unrelated (I see it on other PRs also), but does not cause a failure

Is version 3.22 also fine as a minimum? Otherwise, I'll push a commit that tries bumping the minimum CMake version of that CI run.

DerNils-git · 2024-01-17T16:44:09Z

Yes, sorry I was confused by the first red section in that action.

CMake 3.24 was the lowest version which still works with the implementation that I provided here. Using a lower CMake Version requires to remove the OVERIDE_FIND_PACKAGE argument in FetchContent.

franzpoeschel · 2024-01-18T15:59:17Z

It seems like we need to consider saying good-bye to the Win32 CI runner. The last updates in https://repo.anaconda.com/pkgs/main/win-32/ were in 2022, the latest included CMake version is 3.22.1 from Dec 7, 2021, hence the failure.

franzpoeschel · 2024-01-19T15:14:13Z

Using a lower CMake Version requires to remove the OVERIDE_FIND_PACKAGE argument in FetchContent.

After an offline discussion, this might be the preferred way to go. Even if Win32 is end of life, the runners are still useful as they are a good way to check our handling of datatypes, also Win32 is still being used in lab settings.
Given that this is a program library. we should be careful about raising the required CMake version to something released only very recently, if it can be avoided.

Revert previous commit to use cmake 3.22 in Win32 CI runner.

This reverts commit 5976cc9.

franzpoeschel · 2024-01-23T13:35:58Z

Is it possible to trigger the failed CI again. In the last run it seems that there was an internal error since the directory in which openPMD should have been placed already existed.

I think the Appveyor part of our CI has no way to trigger a selective restart
I force-pushed to give it a second chance

DerNils-git · 2024-01-24T15:10:20Z

@ax3l is there any chance to get direct access to the system on which the CI is failing? That would be easier than debugging this error by additional commits to this pull request.
Unfortunately I could not reproduce this error locally.

franzpoeschel · 2024-01-25T12:35:50Z

I can reproduce the same error an a Linux system using CMake 3.22.1
This might be easier for investigating, I'll have a look

franzpoeschel · 2024-01-25T12:54:46Z

The problem is that CMake 3.22 does not correctly interpret the SOURCE_DIR param on FetchContent_Declare, leaving empty directories. This leads ot a rather cryptic failure in FetchContent_MakeAvailable.
Seems fixed with CMake 3.23. I'd say, we can either use the SOURCE_DIR param only in CMake 3.23 or later, alternatively leave it out altogether.

DerNils-git · 2024-01-25T20:56:45Z

We could also just depricate the SOURCE_DIR keyword. I hoped that FetchContent skips the download if the directory is already populated. Unfortunately this does not work. Therefore adding the SOURCE_DIR does not give any advantage and we could also just use the CMake default directories for FetchContent.

…nt_new

Dependent projects might have the same dependencies and build as a superbuild as well.

ax3l · 2024-09-05T20:58:59Z

Thanks a lot, @DerNils-git!

I'll merge this and do a few follow-up PRs:

remove the files in thirdparty separately Remove ThirdParty Directories #1666
add support for local source directories CMake Superbuild: Repo & Local Source #1667
harmonize further with my usual logic ([1], [2], [3]) CMake Superbuild: Repo & Local Source #1667

One thing I really miss in FetchContent is that it seems to lack support for shallow git clones, which can be a major time sink for large repos. If someone finds an option for this please let me know and we will add it (aka git clone --depth 1).

Alternatively, we could pull a tarball from the releases page of projects... #1668
https://cmake.org/cmake/help/latest/module/FetchContent.html#overview

DerNils-git and others added 20 commits May 24, 2023 10:09

Remove specific version from openPMD json find_package() command

b05ad8f

Merge branch 'openPMD:dev' into dev

8f7e3e8

Replace internal dependency on nlohmann_json by FetchContent

620d861

Update required cmake version for fetchContent and add openPMD interf…

6136a97

…ace library for nlohmann_json

Add nlohmann_json inculde directories

41a4658

Revert "Add nlohmann_json inculde directories"

dc57e24

This reverts commit 41a4658.

Revert "Update required cmake version for fetchContent and add openPM…

74fe22c

…D interface library for nlohmann_json" This reverts commit 6136a97.

Revert "Replace internal dependency on nlohmann_json by FetchContent"

78448ea

This reverts commit 620d861.

Merge branch 'openPMD:dev' into add_cmake_fetchContent

803c359

Replace internal JSON by CMake FetchContent command.

0ba14dc

Replace the old json directory by FetchContent data.

d8cda47

Add the directory filled by FetchContent to .gitignore Add CMakeUserPresets.json to .gitignore

Replace internal TOML11 library by a FetchContent call

7d90228

Replace internal Catch2 library by a FetchContent call

1402795

Update versions within FetchContent to stay with the latest version

2557842

Set C++ Standard to 17 and do not move Catch2 to v3 yet.

8d7dbd6

Remove version updates. Update README.md

0e624d4

Replace PyBind11 dependency by FetchContent

73189c5

Fix pybind11 not found

1c71d9a

Remove pybind11 from repo. Add pybind11 path to gitignore. Simplify p…

0084e9b

…ybind11 configuration.

[pre-commit.ci] auto fixes from pre-commit.com hooks

a4ab2b5

for more information, see https://pre-commit.ci

franzpoeschel reviewed Jan 12, 2024

View reviewed changes

DerNils-git and others added 2 commits January 16, 2024 10:39

Correct Download message for Catch2

acc6e07

Co-authored-by: Franz Pöschel <[email protected]>

More detailed description of FetchContent in README.md

5c8f968

Request cmake>=3.24 in appveyor CI build

5976cc9

Remove OVERRIDE_FIND_PACKAGE to reduce cmake to version 3.22.

cbdcc1c

Revert previous commit to use cmake 3.22 in Win32 CI runner.

Revert "Request cmake>=3.24 in appveyor CI build"

ea5b8db

This reverts commit 5976cc9.

franzpoeschel force-pushed the add_cmake_fetchContent_new branch from c2cf2c9 to ea5b8db Compare January 23, 2024 13:35

Change to trigger CI.

16a9928

Use SOURCE_DIR only in CMake >= 3.23

0f280e2

ax3l self-assigned this Jan 30, 2024

ax3l added this to the 0.16.0 milestone Jan 30, 2024

ax3l added the install label Jan 30, 2024

ax3l force-pushed the dev branch from b80dfee to 805c760 Compare May 7, 2024 17:28

franzpoeschel mentioned this pull request Jul 11, 2024

Release notes: 0.16 #1648

Merged

3 tasks

ax3l added 4 commits September 5, 2024 11:25

Merge remote-tracking branch 'mainline/dev' into add_cmake_fetchConte…

63f239a

…nt_new

Keep Thirdparty in this PR

7b8c6ed

Doc: CMake 3.22.0+

a87abab

Keep Explicit Superbuild Control

fc87c50

ax3l force-pushed the add_cmake_fetchContent_new branch from 638e6b4 to 9d402b5 Compare September 5, 2024 19:12

FetchContent: Repo & Branch/Tag Control

c04fe38

ax3l force-pushed the add_cmake_fetchContent_new branch 2 times, most recently from 580f5ff to 289c872 Compare September 5, 2024 20:35

ax3l added 2 commits September 5, 2024 13:38

Nested Superbuild Support

4d77402

Dependent projects might have the same dependencies and build as a superbuild as well.

Update .gitignore

cce18a8

ax3l force-pushed the add_cmake_fetchContent_new branch from 289c872 to cce18a8 Compare September 5, 2024 20:38

ax3l merged commit 570f155 into openPMD:dev Sep 5, 2024
31 checks passed

This was referenced Sep 5, 2024

Remove ThirdParty Directories #1666

Merged

CMake Superbuild: Repo & Local Source #1667

Merged

ax3l mentioned this pull request Sep 17, 2024

CMake Superbuild: Tarball #1668

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace internal dependencies by FetchContent #1583

Replace internal dependencies by FetchContent #1583

DerNils-git commented Jan 6, 2024 •

edited

Loading

franzpoeschel Jan 12, 2024

DerNils-git Jan 16, 2024

franzpoeschel Jan 17, 2024

ax3l Sep 5, 2024

franzpoeschel Jan 12, 2024

DerNils-git Jan 16, 2024 •

edited

Loading

franzpoeschel Jan 17, 2024

DerNils-git Jan 17, 2024

DerNils-git commented Jan 17, 2024

franzpoeschel commented Jan 17, 2024

DerNils-git commented Jan 17, 2024

franzpoeschel commented Jan 18, 2024

franzpoeschel commented Jan 19, 2024

franzpoeschel commented Jan 23, 2024

DerNils-git commented Jan 24, 2024

franzpoeschel commented Jan 25, 2024

franzpoeschel commented Jan 25, 2024

DerNils-git commented Jan 25, 2024

ax3l commented Sep 5, 2024 •

edited

Loading

Replace internal dependencies by FetchContent #1583

Replace internal dependencies by FetchContent #1583

Conversation

DerNils-git commented Jan 6, 2024 • edited Loading

franzpoeschel Jan 12, 2024

Choose a reason for hiding this comment

DerNils-git Jan 16, 2024

Choose a reason for hiding this comment

franzpoeschel Jan 17, 2024

Choose a reason for hiding this comment

ax3l Sep 5, 2024

Choose a reason for hiding this comment

franzpoeschel Jan 12, 2024

Choose a reason for hiding this comment

DerNils-git Jan 16, 2024 • edited Loading

Choose a reason for hiding this comment

franzpoeschel Jan 17, 2024

Choose a reason for hiding this comment

DerNils-git Jan 17, 2024

Choose a reason for hiding this comment

DerNils-git commented Jan 17, 2024

franzpoeschel commented Jan 17, 2024

DerNils-git commented Jan 17, 2024

franzpoeschel commented Jan 18, 2024

franzpoeschel commented Jan 19, 2024

franzpoeschel commented Jan 23, 2024

DerNils-git commented Jan 24, 2024

franzpoeschel commented Jan 25, 2024

franzpoeschel commented Jan 25, 2024

DerNils-git commented Jan 25, 2024

ax3l commented Sep 5, 2024 • edited Loading

DerNils-git commented Jan 6, 2024 •

edited

Loading

DerNils-git Jan 16, 2024 •

edited

Loading

ax3l commented Sep 5, 2024 •

edited

Loading